Thanks for the review, @olaservo! Good catch on the euc-kr test — you're right, it was silently encoding as UTF-8. Fixed in 37da471: the test now actually calls text.encode("euc-kr") and uses a longer sample so charset-normalizer's statistical detection has enough bytes to work with. This gives us genuine coverage of the detection path for Korean rather than a duplicate of test_utf8_passthrough. All 5 TestGetResponseText tests still pass locally.

@claude review

Claude finished @cliffhall's task in 1m 49s —— View job

PR Review: fix(fetch): detect encoding for non-UTF-8 pages

Tasks:

• Gather context from PR description and comments
• Read changed files
• Review server.py changes
• Review test coverage
• Review dependency changes
• Post final review

This is a solid, well-scoped fix. The fallback chain in get_response_text() is sensible and the previous reviewer's concern about test_korean_euc_kr has been addressed. A few observations follow.

server.py

get_response_text() (lines 29–38) — looks correct

The three-level fallback chain is appropriate:

1. Declared charset in HTTP header → use httpx's native decoding (response.text)
2. No declared charset → run charset_normalizer.from_bytes statistical detection
3. Detection fails → fall back to response.text (httpx applies its own heuristics)

response.charset_encoding returns None only when there is no charset= parameter in the Content-Type header, which is exactly the condition you want. API usage of from_bytes(...).best() and str(result) is correct.

Inconsistency: robots.txt still uses response.text directly (line 106)

# check_may_autonomously_fetch_url — line 106
robot_txt = response.text

This is probably fine in practice (robots.txt is defined to be ASCII), but it's a minor inconsistency with the new approach. A short comment explaining why the standard path is acceptable here would add clarity, or you could use get_response_text(response) for uniformity. Fix this →

TestFetchUrl mocks are unintentionally compatible with the new code

The existing TestFetchUrl tests use MagicMock() responses. MagicMock().charset_encoding auto-creates a truthy MagicMock object (not None), so get_response_text takes the first branch and calls response.text — which the tests have explicitly set. This works, but it's fragile. If charset_encoding were ever set to None in one of those mocks, get_response_text would then call from_bytes(response.content) on another MagicMock, likely producing surprising results. Not a blocker, but worth noting.

tests/test_server.py

Using real httpx.Response objects in TestGetResponseText rather than mocks is exactly the right call — it exercises the actual encoding/decoding path with no stubbing. The _build_response helper is clean.

After the fix in 37da471, test_korean_euc_kr now properly calls .encode("euc-kr") and uses a sufficiently long sample for statistical detection. All five encoding scenarios are covered with realistic, semantically meaningful content.

pyproject.toml

requests is now an unused dependency

requests>=2.32.3 is listed in dependencies, but import requests does not appear anywhere in the server code. The PR description notes charset-normalizer was "already a transitive dep via requests" — now that it's a direct dependency, requests can be removed to reduce the dependency footprint. Fix this →

Summary

The two items worth addressing before merge are removing the unused requests dependency and the robots.txt inconsistency; everything else is a minor nit. Good work on the fix overall.